stabilise fn_args_density by scampi · Pull Request #3581 · rust-lang/rustfmt

scampi · 2019-05-22T20:54:48Z

I think fn_args_density passes the conditions to be made stable.

Is the default value correct ?

Tall is the best compromise for formatting arguments by default:

horizontally when there is enough space
vertically otherwise

The design and implementation of the option are sound and clean.

An enum Density is used so it is easy to add a new kind of density later if needed without breaking backward compatibility. That enum is mapped internally to another enum called ListTactic, the core logic is limited to the function lists::definitive_tactic which looks clear.

The enum Density is used to rewrite where clauses which should probably be decoupled. However, this doesn't prevent stabilisation: it looks like it can be replaced straightforwardly by a boolean though:

src/items.rs
703-            &generics.where_clause,
704-            context.config.brace_style(),
705-            Shape::legacy(where_budget, offset.block_only()),
706:            Density::Vertical,
707-            "{",
708-            where_span_end,
709-            self_ty.span.hi(),
--
1045-
1046-        // Rewrite where-clause.
1047-        if !generics.where_clause.predicates.is_empty() {
1048:            let where_density = if context.config.indent_style() == IndentStyle::Block {
1049:                Density::Compressed
1050-            } else {
1051:                Density::Tall
1052-            };
1053-
1054-            let where_budget = context.budget(last_line_width(&result));
--
1063-                &generics.where_clause,
1064-                context.config.brace_style(),
1065-                Shape::legacy(where_budget, offset.block_only()),
1066:                where_density,
1067-                "{",
1068-                None,
1069-                pos_before_where,
--
1382-                &generics.where_clause,
1383-                context.config.brace_style(),
1384-                Shape::legacy(where_budget, offset.block_only()),
1385:                Density::Compressed,
1386-                ";",
1387-                None,
1388-                body_hi,
--
1459-        &generics.where_clause,
1460-        context.config.brace_style(),
1461-        Shape::legacy(where_budget, indent),
1462:        Density::Vertical,
1463-        "=",
1464-        None,
1465-        generics.span.hi(),
--
2211-        where_clause,
2212-        context.config.brace_style(),
2213-        Shape::indented(indent, context.config),
2214:        Density::Tall,
2215-        "{",
2216-        Some(span.hi()),
2217-        pos_before_where,
--
2322-        &arg_items,
2323-        context
2324-            .config
2325:            .fn_args_density()
2326-            .to_list_tactic(arg_items.len()),
2327-        Separator::Comma,
2328-        one_line_budget,
--
2564-    where_clause: &ast::WhereClause,
2565-    brace_style: BraceStyle,
2566-    shape: Shape,
2567:    density: Density,
2568-    terminator: &str,
2569-    span_end: Option<BytePos>,
2570-    span_end_before_where: BytePos,
--
2646-    } else {
2647-        terminator.len()
2648-    };
2649:    if density == Density::Tall
2650-        || preds_str.contains('\n')
2651-        || shape.indent.width() + " where ".len() + preds_str.len() + end_length > shape.width
2652-    {
--
2737-            &generics.where_clause,
2738-            brace_style,
2739-            Shape::legacy(budget, offset.block_only()),
2740:            Density::Tall,
2741-            "{",
2742-            Some(span.hi()),
2743-            span_end_before_where,

The option is well tested, both in unit tests and, optimally, in real usage.

Plenty of time passed with rustfmt being released with that option, giving it plenty of real usage:

Vertical variant got added 3 years ago db9d129
Compressed and Tall were added 4 years ago 89cda8d

In that time frame, it got only 19 issues showing its implementation is quite stable.

Also, there are few tests about all 3 variants of Density:

tests/target/fn-custom.rs
1:// rustfmt-fn_args_density: Compressed

tests/target/fn_args_density-vertical.rs
1:// rustfmt-fn_args_density: Vertical

tests/target/configs/fn_args_density/tall.rs
1:// rustfmt-fn_args_density: Tall

tests/target/configs/fn_args_density/compressed.rs
1:// rustfmt-fn_args_density: Compressed

tests/target/configs/fn_args_density/vertical.rs
1:// rustfmt-fn_args_density: Vertical

tests/target/fn-custom-7.rs
2:// rustfmt-fn_args_density: Vertical

There is no open bug about the option that prevents its use.

None.

Close #3375

topecongiro · 2019-05-27T02:03:56Z

Seems reasonable to me. My concern is its name, density sounds odd. I prefer to rename it to fn_args_layout.

scampi · 2019-05-27T08:01:07Z

@topecongiro Actually sounds good to me! It reminds me of css flex so using a similar term gives a good idea of what the option does.
@softprops @aldanor you on board with this ?

aldanor · 2019-05-28T00:13:41Z

@scampi Looks good!

fn_args_layout seems reasonable, one question though - in case it's renamed, would fn_args_density become an invalid option?

scampi · 2019-05-28T05:35:20Z

in case it's renamed, would fn_args_density become an invalid option?

Yes, that's the price of an unstable option. If it were stable, that'd be a totally different story!

scampi · 2019-05-30T07:58:42Z

@topecongiro it has been renamed in the latest commit

src/config/options.rs

scampi · 2019-05-30T10:24:46Z

It keeps on failing with the error below, but locally it builds fine:

error[E0433]: failed to resolve: use of undeclared type or module `Density`
    --> src/items.rs:1170:13
     |
1170 |             Density::Compressed,
     |             ^^^^^^^ use of undeclared type or module `Density`
error: aborting due to previous error
For more information about this error, try `rustc --explain E0433`.
error: Could not compile `rustfmt-nightly`.

src/items.rs

unnecessary condition

The density parameter of items::rewrite_where_clause was used only in a conditional, which evaluated to true only if the parameter was Density::Tall.

…hange

…here clauses

topecongiro · 2019-06-03T13:26:40Z

Thank you for the updates!

softprops · 2019-06-03T13:49:11Z

@topecongiro How soon before we could see this in a release?

Also update the rustfmt option fn_args_density to fn_args_layout. See rust-lang/rustfmt#3581. type_alias_enum_variants is apparently now in stable, so there's no need to require it as a feature.

See rust-lang/rustfmt#3581 for background.

See rust-lang/rustfmt#3581 for background. Signed-off-by: Christopher Maier <cmaier@chef.io>

See rust-lang/rustfmt#3581 for background.

See rust-lang/rustfmt#3581 for background. Signed-off-by: Christopher Maier <cmaier@chef.io>

scampi changed the title ~~I think fn_args_density passes the conditions to be made stable.~~ stabilise fn_args_density May 22, 2019

scampi requested a review from topecongiro May 22, 2019 20:55

scampi added this to the 1.3.0 milestone May 22, 2019

scampi force-pushed the issue-3375 branch from 6f7f126 to e305416 Compare May 30, 2019 08:01

topecongiro suggested changes May 30, 2019

View reviewed changes

src/config/options.rs Outdated Show resolved Hide resolved

topecongiro reviewed May 30, 2019

View reviewed changes

src/items.rs Show resolved Hide resolved

scampi added 9 commits May 30, 2019 16:15

Improve documentation of the Density::Tall variant and remove

74259ff

unnecessary condition

replace where density with the boolean on_new_line

7cd7f52

The density parameter of items::rewrite_where_clause was used only in a conditional, which evaluated to true only if the parameter was Density::Tall.

stabilise fn_args_density configuration option

43e0a93

update changelog with new stabilisation

2a915bb

rename fn_args_density configuration option to fn_args_layout

2a35604

update description of the fn_args_layout option to reflect the name c…

e540ead

…hange

rename the fn_args_layout associated enum from Density to ItemsLayout

7c08d37

revert renaming of the Density enum and add a FIXME comment instead

b4650ec

fix rewrite_where_clause call added for handling trait aliases with w…

892d102

…here clauses

scampi force-pushed the issue-3375 branch from c770115 to 892d102 Compare May 30, 2019 14:20

scampi mentioned this pull request Jun 2, 2019

[unstable option] imports_layout #3361

Open

topecongiro merged commit e6b60a4 into rust-lang:master Jun 3, 2019

scampi deleted the issue-3375 branch June 3, 2019 13:35

christophermaier added a commit to habitat-sh/habitat that referenced this pull request Jul 31, 2019

fn_args_density is stabilized as fn_args_layout

c3d7822

See rust-lang/rustfmt#3581 for background.

christophermaier added a commit to habitat-sh/habitat that referenced this pull request Jul 31, 2019

fn_args_density is stabilized as fn_args_layout

61abf1f

See rust-lang/rustfmt#3581 for background. Signed-off-by: Christopher Maier <cmaier@chef.io>

christophermaier added a commit to habitat-sh/builder that referenced this pull request Jul 31, 2019

fn_args_density is stabilized as fn_args_layout

cfc0e77

See rust-lang/rustfmt#3581 for background.

christophermaier added a commit to habitat-sh/builder that referenced this pull request Jul 31, 2019

fn_args_density is stabilized as fn_args_layout

4a76cfa

See rust-lang/rustfmt#3581 for background. Signed-off-by: Christopher Maier <cmaier@chef.io>

christophermaier added a commit to habitat-sh/habitat that referenced this pull request Aug 5, 2019

fn_args_density is stabilized as fn_args_layout

24385b7

See rust-lang/rustfmt#3581 for background. Signed-off-by: Christopher Maier <cmaier@chef.io>

christophermaier added a commit to habitat-sh/habitat that referenced this pull request Aug 5, 2019

fn_args_density is stabilized as fn_args_layout

9840aad

See rust-lang/rustfmt#3581 for background. Signed-off-by: Christopher Maier <cmaier@chef.io>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stabilise fn_args_density#3581

stabilise fn_args_density#3581
topecongiro merged 9 commits intorust-lang:masterfrom
scampi:issue-3375

scampi commented May 22, 2019 •

edited

Loading

Uh oh!

topecongiro commented May 27, 2019

Uh oh!

scampi commented May 27, 2019

Uh oh!

aldanor commented May 28, 2019

Uh oh!

scampi commented May 28, 2019 •

edited

Loading

Uh oh!

scampi commented May 30, 2019

Uh oh!

Uh oh!

scampi commented May 30, 2019

Uh oh!

Uh oh!

topecongiro commented Jun 3, 2019

Uh oh!

softprops commented Jun 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

scampi commented May 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

topecongiro commented May 27, 2019

Uh oh!

scampi commented May 27, 2019

Uh oh!

aldanor commented May 28, 2019

Uh oh!

scampi commented May 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scampi commented May 30, 2019

Uh oh!

Uh oh!

scampi commented May 30, 2019

Uh oh!

Uh oh!

topecongiro commented Jun 3, 2019

Uh oh!

softprops commented Jun 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

scampi commented May 22, 2019 •

edited

Loading

scampi commented May 28, 2019 •

edited

Loading